router: match longest entrypoint path prefix#389
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @avinxshKD! It looks like this is your first PR to volcano-sh/agentcube 🎉 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the upstream URL determination logic to match the longest path prefix among sandbox entrypoints and introduces a helper function for path boundary matching, along with unit tests. The review feedback suggests improving the robustness of the path matching logic by normalizing leading and trailing slashes, and adding a defensive nil check for the sandbox parameter to prevent potential nil pointer dereferences.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Hey @LiZhenCheng9527 @hzxuzhonghu checked this while looking at the router path selection logic. Current code picks the first HasPrefix match, so /can steal /api/foo depending on spec order. Also /api2 matches /api. Pls take a look when free. |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
===========================================
+ Coverage 47.57% 58.76% +11.19%
===========================================
Files 30 36 +6
Lines 2819 3475 +656
===========================================
+ Hits 1341 2042 +701
+ Misses 1338 1225 -113
- Partials 140 208 +68
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
acb3961 to
9b99ab4
Compare
| }, | ||
| wantHost: "10.0.0.2:8080", | ||
| }, | ||
| { |
There was a problem hiding this comment.
This test only covers the boundary case when a / entrypoint exists. With only /api configured, /api2/run still falls through to the first entrypoint, so it can still route to /api; is that fallback behavior intended?
There was a problem hiding this comment.
i agree, added a case for that.
With only /api configured, /api2/run does not match /api, it falls back to the first entrypoint, same as the existing router behavior. This change only affects which matched prefix wins. PTAL
Signed-off-by: Avinash Kumar Deepak <avinash8655279@gmail.com>
9b99ab4 to
0e84798
Compare
|
/lgtm |
/kind bug
What this PR does / why we need it:
Fixes router entrypoint selection when multiple
pathPrefixvalues overlap.The router was picking the first prefix match, so a runtime with
/before/apicould route/api/footo the/entrypoint. It also treated/api2as matching/api.This changes the selection to use the longest valid prefix match and keeps path boundaries sane.
Which issue(s) this PR fixes:
Fixes #388
Special notes for your reviewer:
Added focused tests for overlapping prefixes and
/api2boundary behavior.Does this PR introduce a user-facing change?: